-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tag support to LogGroup resource #53
Conversation
Thanks for your contribution. We are prioritizing this PR review. |
+1 on this PR, we're awaiting this functionality 💯 |
@@ -128,4 +159,19 @@ static String buildResourceDoesNotExistErrorMessage(final String resourceIdentif | |||
ResourceModel.TYPE_NAME, | |||
resourceIdentifier); | |||
} | |||
|
|||
static Map<String, String> translateTagsToSdk(final Set<Tag> tags) { | |||
if (tags == null || tags.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tags.isEmpty()
check is redundant. It should be handled by the stream()
call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will result in same error as below
|
||
static Map<String, String> translateTagsToSdk(final Set<Tag> tags) { | ||
if (tags == null || tags.isEmpty()) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of returning null can we return an empty map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I don't return null
here some of the contract tests will fail with
{'status': 'FAILED', 'errorCode': 'GeneralServiceException', 'message': "1 validation error detected: Value '{}' at 'tags' failed to satisfy constraint: Member must have length greater than or equal to 1 (Service: CloudWatchLogs, Status Code: 400, Request ID: 710f70c1-0dad-4f89-91ad-3eadd83b6e9c, Extended Request ID: null)", 'callbackDelaySeconds': 0}
ResourceModel.TYPE_NAME, model.getLogGroupName(), tagsToAdd); | ||
logger.log(message); | ||
} | ||
} catch (final ResourceNotFoundException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether we need to catch a few more exceptions https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_TagResource.html#API_TagResource_Errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aws-cloudformation/cloudformation-cli-java-plugin#306 should handle AwsServiceExceptions
automatically but the contract seems to still require some to be explicitly mapped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added one more exception handling, because the link provided is actually the wrong one, it is for CloudWatch not CloudWatch Logs. That's the correct one 😄
https://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_TagLogGroup.html#API_TagLogGroup_Errors
aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/UpdateHandler.java
Outdated
Show resolved
Hide resolved
aws-logs-loggroup/pom.xml
Outdated
@@ -43,7 +43,7 @@ | |||
<dependency> | |||
<groupId>software.amazon.awssdk</groupId> | |||
<artifactId>cloudwatchlogs</artifactId> | |||
<version>2.13.18</version> | |||
<version>2.15.26</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There have been issues with resource providers using newer AWS SDK versions than the Java plugin is using:
aws-cloudformation/cloudformation-cli-java-plugin#267 (comment)
aws-cloudformation/cloudformation-cli-java-plugin#324
Is the current AWS SDK version the Java plugin is using sufficient?
<version>2.15.26</version> | |
<version>2.15.19</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, changed it and moved the version to the properties section
@PatMyron is there anything else I should change or more feedback? |
try { | ||
response = proxy.injectCredentialsAndInvokeV2(Translator.translateToReadRequest(model), | ||
ClientBuilder.getClient()::describeLogGroups); | ||
ClientBuilder.getClient()::describeLogGroups); | ||
tagsResponse = proxy.injectCredentialsAndInvokeV2(Translator.translateToListTagsLogGroupRequest(model.getLogGroupName()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To have backward compatibility, the ListTagsLogGroups call should be soft-fail if no permission for tagging. Basically catch the permission error of this API call and ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I can add it, but I don't get why I should support here backward compatibility? Shouldn't I then do it everywhere, where I interact with tags in some way? I'm just really curious about it, hope to get some more insights 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
particularly issues with ReadHandlers
since permissions expand without customer changes. We have an internal issue to eventually find a platform-wide solution to this problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gruebel Some users may not have tagging permissions while creating the log group resource before, especially they use stack role and restrict the permissions to operate a stack .
In this way, it will break people if this change go out. It makes senses to fail if users specify tags during create or update time. However, if they creates the log group successfully before, it will be bad experience to break them. BTW, the READ will be used for GetAtt
function to get the attribute data as well as for drift detection for the resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the insight. Makes sense with the stach role permission for stack updates. I also used it in my previous work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm still curious about is, what is actually the List handler used for. The Read one I understand and it is used also during stack resource import, but the List one?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This List handler is not used in CloudFormation yet, it intends for future features, so having it hard fail for permission issue is right to do.
final ResourceModel model = request.getDesiredResourceState(); | ||
final ResourceModel previousModel = request.getPreviousResourceState(); | ||
final boolean retentionChanged = ! retentionUnchanged(previousModel, model); | ||
final boolean kmsKeyChanged = ! kmsKeyUnchanged(previousModel, model); | ||
final boolean tagsChanged = ! tagsUnchanged(previousModel, model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use getDesiredResourceTags
which will combine both stack tags and resource tag. And getPreviousResourceTags
for both previous stack tags and resource tags.
You will need to override the resourceDefinedTags
(example) so that the getDesiredResourceTags
will merge the stacks and resource defined tags together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, makes sense and will try to add it.
@wbingli would be great to get some feedback for my new changes 😄 |
} | ||
} catch (final ResourceNotFoundException e) { | ||
throwNotFoundException(model); | ||
} catch (final InvalidParameterException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to soft fail the Tag permission as well for Update and Create. The reason is that the Tagging change would comes from stack tags as well. The scenario is that some users don't give Tag permission to the stack. Before this change, the Log group resource can be update successful while doing stack tag update, as this resource just ignore any tag update.
With this change, it will now update the tag for cloudformation stack tags and it can run into permission error and fails the resource update. Soft fail with it as same as read handler can avoid breaking change to those users.
aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/CreateHandler.java
Show resolved
Hide resolved
aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/Configuration.java
Show resolved
Hide resolved
Is there any update on this PR? We are eagerly awaiting this functionality. |
Any update on this PR? |
Any update on this? |
+1 |
It'd be great to have input from either the reviewers or @gruebel (who has already put in a lot of work here, thanks!) about whether help from the community is desired to get this over the finish line. This is a very important feature improvement. |
+1 |
@sfbaker7 Any comment to this PR? I have only one comment here (#53 (comment)) to address. |
sorry, everyone. I totally forgot about my PR 🙈 @wbingli I added the silent fail behaviour for add and remove tags, when trying to update a log group. Tagging permissions are not needed when creating a log group with tags. |
@gruebel Thanks for your contribution. Will merge it once we have other reviewer approve it. |
I am eagerly awaitiing this being merged so I can tag and find my cfn-init logs easily |
Is this still yet to be deployed? It definitely isn't documented and doesn't work if you specify |
Hi, we are expecting this to be available in all regions by the end of next week. Documentation change will follow after Will update this thread once deployed, apologies for the delay! |
Hello there, any update on this ? |
Any update?? This feature still doesn't work, at least in eu regions.. |
Hi this is Felix from AWS, to add support of tags, our team need to go through security review to release it. We are currently working on the review. |
Hi @felixfang - any updates? |
Hi we are still working on the security review. We are aiming to have it ready in September |
Hi Sam, just and update. The security review is in good progress, we just need some more weeks to close it so we can pull your request into our code. We will try complete Tag support in Oct. Sorry for delaying on this. |
@felixfang any update on this, when this can be released? |
Can we get an update on this please? |
It has been released on November 22 ;) |
Is it just me, or are stack-level tags not always applied to log group resources in the stack? I'm seeing inconsistent results where log groups don't always (but sometimes do) have all the tags that are on the stack. |
I had the same behaviour and the cause was that I had to explicitly create the log group resource in the CF template instead of having the related resource create it (aka, the log group must be part of your stack resources). Maybe you are facing the same issue ? |
CF will only propagate changes. In other words, if you have stack-level tags and they have not changed since this feature was introduced, they will not get propagated to the log groups. For already existing stacks, this was an issue for us since we have some tags that do not regularly change on deployments. (Some, like the Version, do, and these were getting propagated as expected.) To force the tags to get propagated, we had to do some manual deployments setting the tags to a temporary value (note, just omitting them will not "delete" them), and then a subsequent deployment with the correct tag values. |
Thank you, that's exactly what's happening in my case, so I'll have to do some manual shenanigans too unfortunately. |
Fixes aws-cloudformation/cloudformation-coverage-roadmap#77
Description of changes:
I added the possibility to add tags to the LogGroup resource.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.